-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow tonumber to work on strings that contain commas as thousands separators #1102
base: master
Are you sure you want to change the base?
Conversation
…parators. Note that this would need to be adapted for non-English locales.
The Appveyor test failure is due to a problem with the continuous integration system itself (as noted in other pull requests), and not to the changes in my pull request. The tests do pass on my machine. |
I don't really have an opinion for or against this pull request, but note that, as with your other pull request, this can be done from
|
int index_to = 0; | ||
while (input[index_from] != 0) { | ||
output[index_to++] = input[index_from++]; | ||
while (input[index_from] == ',') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leading commas won't be removed. Why remove multiple contiguous commas?
Hi! I think it is is interesting. I'm not sure what's the best thing to do here. As @slapresta says, this can be done in jq code. In general I'd rather jq-code things than C-code them, but here it could be a performance issue. Also, if we're going to allow commas as thousands separators, how much should we care about properly formatted numbers? E.g., is |
@nicowilliams You raise valid questions. My immediate goal was to fix the problem for my immediate purposes. I'll see if I can find some time to come up with a more general solution. @slapresta Thanks for the
|
In my opinion, the existing tonumber should be left as-is, partly for stability, but mainly because the existing definition matches the JSON concept of "number" exactly. Anything else it seems would run into internationalization issues. As far as Anglo-American commas are concerned, it seems to me that it is somewhat arbitrary to remove just one of the commas. Removing commas that are in the wrong position has its disadvantages, but a reasonable generalization of
|
Before:
After: